Skip to content

feat(ws): Notebooks v2 add secrets to workspace creation properties #303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: notebooks-v2
Choose a base branch
from
Open

feat(ws): Notebooks v2 add secrets to workspace creation properties #303

wants to merge 3 commits into from

Conversation

thaorell
Copy link

@thaorell thaorell commented May 1, 2025 •
edited
Loading

closes: #267
related: depends on #262

Note:

  • The modal to create a new secret currently takes in octal value as defaultMode and stores it as decimal before submitting back to API
  • The default mode also includes input validation to ensure the input is a valid unix permission value
2025_05_01-15_35_11.webm

@github-project-automation github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks May 1, 2025
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ederign for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@thaorell thaorell force-pushed the notebooks-v2-workspace-creation-secrets-select branch from 9c0a3ce to de8fe2a Compare May 2, 2025 13:34
paulovmr
paulovmr suggested changes May 6, 2025
View reviewed changes
Copy link

@paulovmr paulovmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thaorell for the PR! I left a few comments, after those are fixed and we get the dependent PR merged and your branch rebased, I can do a final review.

@thaorell thaorell force-pushed the notebooks-v2-workspace-creation-secrets-select branch from de8fe2a to 24c4046 Compare May 6, 2025 14:51
@paulovmr
Copy link

paulovmr commented May 7, 2025

/ok-to-test

@paulovmr paulovmr mentioned this pull request May 8, 2025
Open
@thaorell thaorell force-pushed the notebooks-v2-workspace-creation-secrets-select branch from 24c4046 to 4b7b453 Compare May 8, 2025 15:55
@google-oss-prow google-oss-prow bot added size/L and removed size/XXL labels May 8, 2025
@thaorell
Copy link
Author

thaorell commented May 8, 2025

@paulovmr the only pending items are the messages on the UI. I'll wait for @jenny-s51 's response

@thaorell
Copy link
Author

thaorell commented May 8, 2025

screencast-localhost_9000-2025_05_08-14_02_58.webm

@thaorell thaorell requested review from paulovmr and jenny-s51 May 8, 2025 18:03
@thaorell thaorell force-pushed the notebooks-v2-workspace-creation-secrets-select branch from e9a3245 to 5b3660c Compare May 8, 2025 18:34
paulovmr
paulovmr approved these changes May 8, 2025
View reviewed changes
Copy link

@paulovmr paulovmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label May 8, 2025
jenny-s51
jenny-s51 suggested changes May 8, 2025
View reviewed changes
Copy link
Member

@jenny-s51 jenny-s51 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one quick fix below, otherwise LGTM. Thanks for making these changes @thaorell !

@google-oss-prow google-oss-prow bot removed the lgtm label May 8, 2025
@thaorell thaorell force-pushed the notebooks-v2-workspace-creation-secrets-select branch from 5b3660c to 90dcc40 Compare May 9, 2025 13:13
@thaorell thaorell requested a review from jenny-s51 May 9, 2025 13:14
jenny-s51
jenny-s51 suggested changes May 9, 2025
View reviewed changes
Copy link
Member

@jenny-s51 jenny-s51 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making suggested update Charles !

I'm seeing a slight spacing inconsistency between the expandable section and the description text below. The gap between the Volumes expandable section and text is larger than the gap for Secrets.

Can we apply a layout to make sure the spacing is consistent for both? The Stack layout with gutter would be good to use here . WDYT @thaorell ?
Screenshot 2025-05-09 at 11 20 33 AM

@thaorell thaorell force-pushed the notebooks-v2-workspace-creation-secrets-select branch from 90dcc40 to 13284fc Compare May 9, 2025 17:14
@thaorell
Copy link
Author

thaorell commented May 9, 2025 •
edited
Loading

@jenny-s51 Thank you for catching that. It was due to a wrongly wrapped <div>, and I've corrected it. I do think there's value in using <Stack>, and I think it'd be best to create another issue for that. If you do, please feel free to assign me. What do you think?

Screenshot 2025-05-09 at 1 16 08 PM

@thaorell thaorell requested a review from jenny-s51 May 9, 2025 17:19
jenny-s51
jenny-s51 approved these changes May 9, 2025
View reviewed changes
Copy link
Member

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thaorell ! Latest changes look good to me - yes, let's merge and I can open a follow-up ticket for a couple additional enhancements we'll want to include here:

  1. Applying Stack layout for consistent spacing in these sections.
  2. Persisting the description text when the expandable section is open - currently it gets removed with the !isVolumesExpanded flag however we want to expose it in both cases.

Great work!

@google-oss-prow google-oss-prow bot added the lgtm label May 9, 2025
@jenny-s51 jenny-s51 mentioned this pull request May 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulovmr paulovmr paulovmr approved these changes

@jenny-s51 jenny-s51 jenny-s51 approved these changes

@ederign ederign Awaiting requested review from ederign

@thesuperzapper thesuperzapper Awaiting requested review from thesuperzapper

Assignees

@paulovmr paulovmr

@jenny-s51 jenny-s51

Labels
lgtm ok-to-test size/L
Projects
Kubeflow Notebooks
Status: Needs Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thaorell @paulovmr @jenny-s51